Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updating containsKey method in PassiveExpiringMap #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thePatmanMI
Copy link

correcting the containsKey method to remove all expired entries to match the class and method javadoc statements

correcting the containsKey method to remove all expired entries to match the class and method javadoc statements
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a unit test that fails without this patch. Thank you!

@thePatmanMI
Copy link
Author

are there additional problems with this change, or can it be merged in?

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks OK now I think, with a test. Before being merged, it would require a JIRA ticket as well, to be included in changes.xml.

I had a look at the code in PassiveExpiringMap, and the same removeIfExpired(key, now()) is used in other three methods. The javadocs do not clarify if the key argument is supposed to be used and clear only the expired values with that key, or not.

So for me the options here are to leave as-is and clarify in the Javadoc documentation what is cleared. Otherwise, to modify all methods to match the current javadocs and to expireAll instead.

@garydgregory
Copy link
Member

I am waiting for the comments from @kinow to be resolved, by @thePatmanMI or another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants